Skip to content

fix: retry transient InnoDB deadlocks during option-store lock acquisition - #580

Merged
chubes4 merged 1 commit into
mainfrom
test/576-deterministic-option-store-race
Sep 29, 2026
Merged

chubes4 merged 1 commit into
mainfrom
test/576-deterministic-option-store-race

Conversation

@chubes4

@chubes4 chubes4 commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Fixes #576

Root cause: real store bug, not a test-timing artifact

Diagnosis required actual reproduction, not code-reading alone. I ran tests/run-control-option-store-integration.php against a live, heavily-contended local MariaDB instance (a shared production database, not a clean CI container) and the failure reproduced in roughly 70-80% of runs — far above the "1-in-30" rate reported in the issue, but the same failure mode.

Instrumenting acquire_lock() to log the underlying error on false === $created showed:

DEBUG lock creation failed errno=1213 error=Deadlock found when trying to get lock; try restarting transaction

Two connections racing INSERT IGNORE against the same unique option_name lock row can make InnoDB report a deadlock (1213) instead of the clean "0 rows affected, key already exists" outcome the retry loop is written to expect. acquire_lock() treated any query failure on that statement as fatal and threw WP_Agent_Run_Control_Store_Exception immediately, with no retry — even though a deadlock here is exactly the same admission race the surrounding do {} while loop already retries for the duplicate-key case. The same failure class applies to the conditional takeover UPDATE used for expired-lock recovery.

This directly explains both failing assertions:

  • site option mutex serializes two independent connection mutations — one of the two concurrent mutate_state() calls threw on the deadlock, so only one increment ever landed (final counter = 1, not 2).
  • enqueue concurrent with claim succeeds (the assertion named in Flaky: run-control-option-store-integration 'enqueue concurrent with claim succeeds' #576) — queue_message() catches \RuntimeException and converts it to a WP_Error (agents_chat_run_atomic_unavailable), so the same deadlock surfaces as queued => false.

This is a genuine correctness/robustness gap in the store's own lock acquisition, not a test synchronization problem — the existing fork/ready/go barrier already guarantees real overlap, and the two-worker mutex model is architecturally sound. The bug is that the retry loop didn't recognize a transient storage-engine outcome of its own contention.

Fix

acquire_lock() now treats an INSERT IGNORE (or takeover UPDATE) failure as retryable when $db->last_error indicates a transient InnoDB deadlock or lock-wait timeout, reusing the loop's existing retry/backoff primitive (usleep(50000) + the existing 5s deadline) instead of failing closed immediately. Non-transient failures (real connectivity/schema errors) still throw immediately, unchanged.

private function is_transient_lock_contention( \wpdb $db ): bool {
    return str_contains( $db->last_error, 'Deadlock found' ) || str_contains( $db->last_error, 'Lock wait timeout' );
}

The test file is unchanged and now serves as the regression coverage.

Verification

  • Reproduced pre-fix: 25/30 and then multiple further runs failing locally (site option mutex... and enqueue concurrent with claim succeeds), consistently with errno=1213 on the lock-creation INSERT IGNORE.
  • Post-fix: 50/50 passes of php tests/run-control-option-store-integration.php against the same contended local MariaDB (run in a background loop; each run includes a ~16.5s intentional sleep in the lease-race test, so 50 runs took roughly 14 minutes).
  • composer smoke — pass (exit code 0).
  • composer phpstan — [OK] No errors.

AI disclosure

AI assistance: Yes
Tool: OpenCode
Used for: diagnosis (reproduction, instrumentation, root-cause isolation of the InnoDB deadlock), fix implementation, and test verification (50-run loop, smoke, phpstan).

…ition

Two connections racing an INSERT IGNORE (or a conditional takeover
UPDATE) against the same unique lock row can make InnoDB report a
deadlock (1213) or lock-wait timeout instead of the clean "0 rows
affected" outcome the retry loop already handles. acquire_lock()
treated any query failure as fatal, so a purely transient storage-
engine artifact of the exact admission race it serializes surfaced as
an immediate WP_Agent_Run_Control_Store_Exception instead of a retry.

Reproduced locally against a live, contended MariaDB instance: the
existing 'site option mutex serializes two independent connection
mutations' and 'enqueue concurrent with claim succeeds' assertions in
tests/run-control-option-store-integration.php failed in roughly 70-80%
of runs before this change (confirmed via last_error: 'Deadlock found
when trying to get lock; try restarting transaction') and passed 50/50
after it. Fixes #576.
@chubes4
chubes4 merged commit 79317da into main Sep 29, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky: run-control-option-store-integration 'enqueue concurrent with claim succeeds'

1 participant